feat(sessions): add offset parameter to get_items() for pagination#2878
feat(sessions): add offset parameter to get_items() for pagination#2878m1lestones wants to merge 1 commit intoopenai:mainfrom
Conversation
Adds an optional offset: int = 0 parameter to get_items() across all session backends, enabling callers to paginate backwards through history. session.get_items(limit=10) # 10 most recent (unchanged) session.get_items(limit=10, offset=10) # next 10 older items session.get_items(offset=5) # all except 5 most recent Closes openai#2810 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
bot
added
enhancement
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20308a3b32
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async def get_items( | ||
| self, limit: int | None = None, offset: int = 0 | ||
| ) -> list[TResponseInputItem]: | ||
| encrypted_items = await self.underlying_session.get_items(limit, offset) |
There was a problem hiding this comment.
Forwarding limit and offset positionally here breaks wrapped sessions whose second positional parameter is not offset (notably AdvancedSQLiteSession, where it is branch_id) and can also raise TypeError for custom sessions that still accept only limit or make offset keyword-only. This means EncryptedSession.get_items() can silently fetch the wrong history (often empty) even with default arguments; forward using keywords to preserve compatibility.
Useful? React with 👍 / 👎.
| async def get_items( | ||
| self, limit: int | None = None, offset: int = 0 | ||
| ) -> list[TResponseInputItem]: | ||
| return await self.underlying_session.get_items(limit, offset) |
There was a problem hiding this comment.
This positional forward has the same compatibility regression: wrapped sessions with a non-offset second positional argument (e.g., AdvancedSQLiteSession.branch_id) or older/custom get_items(limit) implementations will misbehave or fail when compaction calls through this path. Because this wrapper is intended to decorate arbitrary Session implementations, offset should be passed by keyword to avoid breaking valid underlying sessions.
Useful? React with 👍 / 👎.
|
Thanks for sharing this. However, as I mentioned at #2811 (comment), we won't have the optional parameter alone as it won't be used in any use cases. |
|
Understood, thank you for the quick feedback. I'll focus on issues with clearer internal use cases. |
2 participants
Summary
Closes #2810
Adds an optional
offset: int = 0parameter toget_items()across all session backends, enabling proper pagination through conversation history.Backends updated:
SQLiteSession—LIMIT ? OFFSET ?in SQL queryAsyncSQLiteSession— same SQL patternSQLAlchemySession—.offset()clauseRedisSession— adjustedlrangenegative index rangeDaprSession— list slicing from newest endEncryptSession— passesoffsetthrough to underlying sessionOpenAIResponsesCompactionSession— passesoffsetthrough to underlying sessionOpenAIConversationsSession— fetcheslimit+offsetin DESC, skips firstoffset, reversesAdvancedSQLiteSession— same SQL pattern,offsetappended afterbranch_idTest plan
test_sqlite_session_get_items_with_offsetcovering offset+limit pagination, offset-only, and out-of-range offsettest_simple_list_session_get_items_offsetfor the in-memory test utility2678 passed, 5 skippedmake format,make lint,make typecheckall cleanoffsetdefaults to0, existing behavior unchanged🤖 Generated with Claude Code